Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

semi-fungible token #349

Merged
merged 35 commits into from
Nov 19, 2021
Merged

semi-fungible token #349

merged 35 commits into from
Nov 19, 2021

Conversation

fiftyeightandeight
Copy link
Contributor

closes #303

@fiftyeightandeight fiftyeightandeight self-assigned this Nov 12, 2021
@SaadTahirTintash
Copy link
Contributor

@fiftyeightandeight Why is the token-wbtc and token-usda set to u6? Shouldn't token-wbtc be u8 and token-usda be u2? Due to u6, I am getting errors in the tests/fixed-weight-pool_test.ts

- Test case files has bugs in imports
- A change in token-wstx and token-usda get-decimals is questionable
- Added mintFixed for now
@fiftyeightandeight
Copy link
Contributor Author

@fiftyeightandeight Why is the token-wbtc and token-usda set to u6? Shouldn't token-wbtc be u8 and token-usda be u2? Due to u6, I am getting errors in the tests/fixed-weight-pool_test.ts

USDA supports up to 6 decimals (https://github.com/arkadiko-dao/arkadiko/blob/master/clarity/contracts/usda-token.clar), hence.

WBTC is a bit arbitrary, but I am thinking about standardising every token to 6 decimals to be consistent with STX (minor point is it might somewhat reduce errors in the last two digit decimals).

@fiftyeightandeight
Copy link
Contributor Author

@SaadTahirTintash once #324 is fixed, please let me know as we need to work through the test cases for this PR.

@fiftyeightandeight fiftyeightandeight linked an issue Nov 15, 2021 that may be closed by this pull request
@SaadTahirTintash
Copy link
Contributor

Okay So I've added the mint function in the model but the test cases are not right for now. I am pushing the code which is subject to change from mintFixed to mint as I am not sure what do we need to use.

Copy link
Contributor

@MarvinJanssen MarvinJanssen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to keep the scope to just reviewing things related to the semi-fungible token implementation. I like how you are using SFTs to capture all yield token variations in a single contract. Very nice.

Biggest thing is that the trait does not confirm to the latest draft of the SIP.

clarity/contracts/key-token/key-usda-wbtc.clar Outdated Show resolved Hide resolved
clarity/contracts/pool-token/ytp-yield-wbtc.clar Outdated Show resolved Hide resolved
@@ -0,0 +1,39 @@
(define-trait semi-fungible-token-trait
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest using the trait from the SIP and then defining custom features in a separate ALEX-specific trait. This trait does not conform to the current draft which could cause issues down the line. (If it is ratified as-is.)

Copy link
Contributor Author

@fiftyeightandeight fiftyeightandeight Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SaadTahirTintash would you please help refactor this trait to reflect @MarvinJanssen 's comments?

@fiftyeightandeight
Copy link
Contributor Author

@MarvinJanssen , as discussed, we will refactor trait to include transfer and transfer-memo (#352 )
Also on token-owned, yes, I agree that this ideally should/could be handled on a backend by indexing the relevant chain events. We will replace the relevant code in the contract when a backend solution is ready.

@fiftyeightandeight fiftyeightandeight marked this pull request as ready for review November 17, 2021 01:49
@MarvinJanssen MarvinJanssen self-requested a review November 17, 2021 09:53
@fiftyeightandeight fiftyeightandeight linked an issue Nov 18, 2021 that may be closed by this pull request
Copy link
Contributor

@MarvinJanssen MarvinJanssen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR feels a little stuffy, many changed files. I limited the review to just the SFT. Looks fine from what I can see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment